Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: node filtering edge rules also filtering out edges #234

Closed
wants to merge 3 commits into from

Conversation

buckhalt
Copy link
Member

@buckhalt buckhalt commented Dec 2, 2024

This PR conceptually changes how node filtering edge rules are implemented.

Previously: edge rules filtered nodes and the edges themselves.

Changed logic: Edge rules filter nodes. Edges themselves are not filtered by node filtering rules. Edges are only trimmed to remove orphaned nodes caused by node filtering.

  • Filter edges by edge rule -- filteredEdges
  • Filter nodes by filteredEdges -- filteredNodes
  • Trim all network edges to remove any orphaned edges caused by removed nodes. This removes edges that make the network invalid. -- validEdges
  • Return filteredNodes and validEdges

Tests added and existing tests updated to cover conceptual changes.

Todo:

  • Update language in documentation article to reflect this conceptual change
  • Make equivalent change in Interviewer

Copy link

vercel bot commented Dec 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fresco-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 8:52pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
barf ⬜️ Ignored (Inspect) Visit Preview Dec 3, 2024 8:52pm

should not expect filtered edges for node filtering
lib/network-query/rules.js Outdated Show resolved Hide resolved
@buckhalt
Copy link
Member Author

buckhalt commented Dec 4, 2024

Closing PR. Team decided to keep filtering logic as-is and update messaging in Architect to communicate more clearly to users.

@buckhalt buckhalt closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants